Order a category listing by the position set in the browsed category - #1289
Open
boo-code wants to merge 1 commit into
Open
Order a category listing by the position set in the browsed category#1289boo-code wants to merge 1 commit into
boo-code wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PS_LAYERED_FULL_TREEthe rows are restricted withnleft/nrightrather than to the browsed category, socp.positionis not functionally dependent on the product the query groups by and the server keeps whichever row it likes. Leaf categories are unaffected, which is why a shop looks correct everywhere except parent categories. The ordering is now pinned to the browsed category, and a product reached only through a subcategory - which has no position there - is listed after the ones the merchant actually arranged.composer test. The added case asserts the generated SQL: the ordering expression is evaluated in the outer query, wherecategory_productis joined, and is kept out of the initial population, which does not join it. Reverting onlysrc/Adapter/MySQL.phpmakes the initial population selectISNULL(MIN(IF(cp.id_category = 6, cp.position, NULL)))against an alias it never joins, which is anUnknown column 'cp.id_category'at runtime. Manually: give products a deliberate order in a category that has subcategories and whose products also sit in those subcategories, turn on "Show products from subcategories", and browse it sorted by position.Why it is a real defect rather than a data quirk
The query is not valid aggregation - it only runs because PrestaShop sets
sql_mode = ''inDbPDO.Asking the same server to apply the standard rejects it outright:
So which position wins is undefined, not merely unexpected. On the default data set of a 9.1 shop the
server happens to keep the browsed category's row and the listing looks right; forcing the other
candidate rows on the same data gives
16,6,17,7,18,8,…where the merchant's order is7,6,8,10,9,11,…. That is the same symptom #42279 reports from a production shop, so this fixes alatent defect whose visibility depends on the plan and the data rather than one that reproduces
everywhere.
The two changes
src/Filters/Products.phppins the ordering:ISNULL(...) ASCkeeps the products that have no position in the browsed category last in bothdirections, which a sentinel value would not do on
DESC.src/Adapter/MySQL.phpstops pushing an expression order field into the initial population.computeOrderByField()already treats a value containing(as an outer-query expression and returnsit untouched, but it had added it to the initial population's select list first. The initial population
does not carry the outer query's joins, so the expression there refers to tables that are not joined at
that level. Any caller passing an expression hits this, not just this one.